@jimtng @mherwege for info..

@jimtng good news that all your PRs have all now been merged; which means that this one is also now ready to go: => could you please have a look at it?

how would it handle the scenario when linked to an item with unit, but the condition is OtherItem1 > OtherItem2

Good question. If I recall correctly I think the code converts 'OtherItem' to the string representation of its State, and then the string is handled in the same way as one hard coded in the condition LHS / RHS. So if 'OtherItem' was a QuantityType we would attempt to convert it to the systemUnit. And in the case 'OtherItem1 < OtherItem2' .. if either or both would not convert to systemUnit we would have an error -- which I guess means that the input State is rejected??

PS in any case I will add some JUnit tests.. => EDIT: I see that there are actually already many such tests .. and they all still pass.

@andrewfg This change broke a comparison between a MQTT number channel with unit kWh and the comparison string < 50, I needed to change this to < 50 kWh.

The logging was

// before:
ant 11:55:37.211 | DEBUG | org.openhab.transform.basicprofiles.internal.profiles.StateFilterProfile | Evaluating Condition(''  LT '50' ) with input: 4.518 kWh (QuantityType). Link: 'PV_H32_Energie_heute -> mqtt:topic:sbfspot_2100026732:EToday'
ant 11:55:37.213 | DEBUG | org.openhab.transform.basicprofiles.internal.profiles.StateFilterProfile | RHS: '50' is not compatible with LHS '4.518 kWh' (QuantityType)
ant 11:55:37.215 | DEBUG | org.openhab.transform.basicprofiles.internal.profiles.StateFilterProfile | Received state update from handler: 4.518 kWh, not forwarded to item

// after:
ant 11:57:36.172 | DEBUG | org.openhab.transform.basicprofiles.internal.profiles.StateFilterProfile | Evaluating Condition(''  LT '50 kWh' ) with input: 4.601 kWh (QuantityType). Link: 'PV_H32_Energie_heute -> mqtt:topic:sbfspot_2100026732:EToday'
ant 11:57:36.188 | DEBUG | org.openhab.transform.basicprofiles.internal.profiles.StateFilterProfile | Performing a comparison between input '16563600 Ws' (QuantityType) and value '180000000 Ws' (QuantityType)
ant 11:57:36.190 | DEBUG | org.openhab.transform.basicprofiles.internal.profiles.StateFilterProfile | Received state update from handler: 4.601 kWh, forwarded as 4.601 kWh

I think it would be good to add a breaking change warning and advice the users to check unit compatibility, WDYT?

@florian-h05 thanks for reporting this. I suppose there are two ways forward..

1. Let comparison targets without a unit be handled as if the decimal value would be in the unit of the target Item.
2. Or as you suggest, mark it as a breaking change.

Probably 2. is the “correct” approach, but 1. is more lenient. Actually I thought that I had programmed it to do case 1. so I need to check. @jimtng any thoughts?

I thought that it required the unit from before this PR.

Prior to this PR, the unit of the linked item didn't matter. It only looked at the incoming value from the binding, which in this case would've been a QuantityType, and it would not allow comparison against a non QuantityType.

Since this PR isn't a patch, you could test it on 4.3.2 (old behaviour prior to this PR)

@jimtng thanks for the feedback. I think there are two topics..

1. The question if the case of @florian-h05 is a breaking change: As I understand you, it was the same behavior prior to this PR so it's not a breaking change. Therefore no need for a warning in the release notes. Or?? However perhaps it means I should tweak the read me to make the use case clearer?

2. The question if we should make another PR for the case where the incoming state is a QuantityType and the comparand has no unit; to assume that the unit is that of the target Item. I am not sure if this is a good idea. => WDYT?

1. The question if the case of @florian-h05 is a breaking change: As I understand you, it was the same behavior prior to this PR so it's not a breaking change.

Correct, but I'm only 99% sure (don't remember exactly). We need feedback from @florian-h05 to see if he could test this on 4.3. I can also test tomorrow (it's very late here right now, I'm ready to hit the hay)

However perhaps it means I should tweak the read me to make the use case clearer?

What do you mean?

2. The question if we should make another PR for the case where the incoming state is a QuantityType and the comparand has no unit; to assume that the unit is that of the target Item. I am not sure if this is a good idea.

I don't think it's a good idea either. If my memory serves, this case was specifically not allowed since my first PR to this
See #17323
This specific test checked for it:

That was in August 2024.

What do you mean?

I was thinking to make it clearer that the data type of the second comparand must match that of the first comparand in the chapter below.

https://www.openhab.org/addons/transformations/basicprofiles/#state-filter-conditions

I was thinking to make it clearer that the data type of the second comparand must match that of the first comparand in the chapter below.

I just had a look at the current doc (https://next.openhab.org) and I agree it isn't clear / emphasised that quantitytype data must be compared against a quantitytype constant / data, and comparison against plain number / non quantity will cause it to fail.